Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(animations): avoid animation insertions during router back/refresh #21977

Closed

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Feb 1, 2018

Closes #19712

@matsko matsko added the target: patch This PR is targeted for the next patch release label Feb 1, 2018
@matsko matsko requested a review from IgorMinar February 1, 2018 22:40
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please adjust the comment before merging

// there may exist a race condition here (within the router functionality)
// that may end up having DOM nodes be inserted prematurely even though
// the new DOM is not ready just yet. This is a router-level issue,
// however animations should be defensive about this situation (issue #21947)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about:

This if statement is a workaround for router issue #21947.
The router sometimes hits a race condition where while a route is being instantiated a new navigation arrives, triggering leave animation of DOM that has not been fully initialized, until this is resolved, we need to handle the scenario when DOM is not in a consistent state during the animation.

Please check if what I wrote is technically correct and makes sense to you. I found your version to be hard to understand.

@IgorMinar IgorMinar added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 1, 2018
@mary-poppins
Copy link

You can preview a1e594c at https://pr21977-a1e594c.ngbuilds.io/.

@matsko matsko force-pushed the animation_insert_race_condition branch from a1e594c to 5f80904 Compare March 22, 2018 20:34
@mary-poppins
Copy link

You can preview 5f80904 at https://pr21977-5f80904.ngbuilds.io/.

@matsko matsko added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 22, 2018
@ngbot
Copy link

ngbot bot commented Mar 22, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "continuous-integration/travis-ci/pr" is failing
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@matsko matsko closed this in f88fba0 Mar 22, 2018
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@matsko matsko deleted the animation_insert_race_condition branch January 17, 2019 19:44
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: animations cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(animations): Cannot read property insertNode of undefined
4 participants